-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Synacormedia: Added video support to adapter. #3695
Conversation
Added playerSize to video test page.
f113a35
to
856a15a
Compare
856a15a
to
1c0eaf0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good, just a few comments/questions
modules/synacormediaBidAdapter.js
Outdated
let size0 = size[0]; | ||
let size1 = size[1]; | ||
let imp = { | ||
id: videoOrBannerKey.substring(0, 1) + bid.bidId + '-' + size0 + 'x' + size1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use template strings if possible in Prebid, so line 68 is changed to something like:
id: `${videoOrBannerKey.substring(0,1)}${bid.bidId}-${size0}x${size1}
`
modules/synacormediaBidAdapter.js
Outdated
@@ -79,41 +100,43 @@ export const spec = { | |||
} | |||
}, | |||
interpretResponse: function(serverResponse) { | |||
var updateMacros = (bid, r) => { | |||
return r ? r.replace(/\${AUCTION_PRICE}/g, parseFloat(bid.price)) : r; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since replace
expects a string type for the replacement argument, the parseFloat will be converted back to a string. Could you remove parseFloat or is it used to format the price?
return r ? r.replace(/\${AUCTION_PRICE}/g, bid.price) : r;
modules/synacormediaBidAdapter.js
Outdated
width: parseInt(width, 10), | ||
height: parseInt(height, 10), | ||
creativeId: seatbid.seat + '~' + bid.crid, | ||
creativeId: seatbid.seat + '_' + bid.crid, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A template string could be used here:
creativeId: `${seatbid.seat}_${bid.crid}`,
modules/synacormediaBidAdapter.js
Outdated
currency: 'USD', | ||
netRevenue: true, | ||
mediaType: BANNER, | ||
mediaType: (isVideo ? VIDEO : BANNER), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a recommendation, but you could simplify by removing the parens here:
mediaType: isVideo ? VIDEO : BANNER,
I have updated the PR and addressed the comments. |
Just bumping this to see if it can be re-reviewed with the recommended changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* Synacormedia: Added video support to adapter. Added playerSize to video test page. * Synacormedia: Updated test case for IE/Edge * Synacormedia: Updates based on PR review
Type of change
Description of change
Added support for Video bids to Synacormedia bid adapter
contact email of the adapter’s maintainer: [email protected]
Documentation PR: Synacormedia: Added video parameters to documentaion. prebid.github.io#1236